ARTEMIS-6162 Only start lock coordinators once the broker is activated#6590
Open
MyLuk wants to merge 2 commits into
Open
ARTEMIS-6162 Only start lock coordinators once the broker is activated#6590MyLuk wants to merge 2 commits into
MyLuk wants to merge 2 commits into
Conversation
| |How often to check if the lock is still valid, in milliseconds | ||
| |=== | ||
|
|
||
| NOTE: A lock coordinator only starts polling for the lock once the broker has actually been activated (i.e. it is the live/primary server, or a backup that has taken over). On a passive HA backup the lock coordinator stays stopped, so it never contends for the lock with the currently active node; it starts polling only once that backup itself activates. |
Contributor
There was a problem hiding this comment.
I foresee a not distant future where people only use mirroring for HA.
Can we improve this note with:
NOTE: Lock coordinators do not poll for the lock when the broker is configured as a backup using shared store or journal replication HA. Polling only begins once the backup activates as the live server.
Contributor
There was a problem hiding this comment.
or even better:
NOTE: In a journal HA setup with either shared storage or journal replication, lock coordinators will stay inactive and will not poll for the lock until the server is activated.```
Author
There was a problem hiding this comment.
Good catch on the scope - the note read like a universal rule. Took your second wording verbatim, thanks. Fixed in 9fba7fc.
Contributor
|
everything looks good to me. Question: did you use AI to write these test? the style of comments left in the test seems a bit Claude to me. |
ActiveMQServerImpl.internalStart() started every configured lock coordinator right away, regardless of the HA role of the node, so a passive backup contended for the lock along with the active primary. Two things go wrong with that. A passive backup that wins the lock holds it while none of its acceptors or broker connections exist yet, so the elements bound to that coordinator are down on every node of the group. And when brokers in different data centers share a lock, the backup of a failed primary needs a few seconds to activate while the remote brokers keep polling every check-period, so the lock usually lands on a remote broker rather than on the backup that is taking over. Split the creation from the start. The coordinators are still created in internalStart(), since acceptors and broker connections look them up by name while they are being created, but polling for the lock now starts in completeActivation(), the single point every activation goes through, right before the acceptors are started. Only a node that actually became active contends for the lock. Added LockCoordinatorHaActivationTest: a replicated pair sharing one coordinator. The passive backup keeps its coordinator stopped while the primary holds the lock, and takes the lock over once the primary is gone. Without the fix the backup is polling for the lock while still passive and the test fails. Assisted-by: Claude
…nators Now that lock coordinators only start once a broker is activated, the brokers that are active still all race for the lock. When a primary fails, a broker in a standby data center can grab the lock before the backup of that primary has finished activating, so the group comes back in the wrong place. Which brokers may take a lock over on their own is a deployment decision, not something a broker can work out by itself. Added an auto-start attribute on lock-coordinator, defaulting to true so nothing changes for existing configurations. With auto-start=false the coordinator is created and visible in management, but it never polls for the lock on its own, so it stays out of the race until somebody decides otherwise. To make that decision possible, ActiveMQServerControl now exposes startLockCoordinator and stopLockCoordinator next to the existing listLockCoordinatorsAsJSON, both audited. Starting an already started coordinator does nothing, stopping one that holds the lock releases it and pauses whatever is bound to it, and an unknown name is rejected. A coordinator can be started again after it was stopped, so a data center can be handed the lock and handed it back. Added LockCoordinatorAutoStartManagementTest for the whole cycle on an acceptor bound to an auto-start=false coordinator, a unit test for the flag itself and parser coverage for the new attribute. Assisted-by: Claude
Author
|
Yes, these tests were written with Claude's assistance - I reviewed them and confirmed both fail without the respective fix. I wasn't aware of the Assisted-by recommendation, thanks for pointing it out - added Assisted-by: Claude to both commit messages and force-pushed. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two commits, one per JIRA:
ARTEMIS-6162 -
internalStart()started every configured lock coordinator right away, whatever the HA role of the node, so a passive backup contended for the lock along with the active primary. A passive backup that wins the lock holds it while none of its acceptors or broker connections exist yet, so the elements bound to that coordinator are down on every node of the group. And with brokers in different data centers sharing a lock, the backup of a failed primary needs a few seconds to activate while remote brokers keep polling every check-period, so the lock usually lands on a remote broker rather than on the backup taking over.The coordinators are still created in
internalStart(), since acceptors and broker connections look them up by name while being created, but polling now starts incompleteActivation(), right before the acceptors are started.ARTEMIS-6163 - even then, all active brokers still race for the lock, and which of them may take a lock over on its own is a deployment decision. Adds an
auto-startattribute onlock-coordinator(defaulttrue, no behaviour change) plusstartLockCoordinator/stopLockCoordinatormanagement operations, both audited, so a standby data center can be configured withauto-start=falseand handed the lock only when somebody decides so.Both behaviours were suggested by @clebertsuconic on the users list ("Artemis Cluster with HA and with Mesh Mirror", May 2026).
Tests:
LockCoordinatorHaActivationTest(replicated pair, the passive backup stays out of the lock and takes it over once the primary is gone) andLockCoordinatorAutoStartManagementTest(full start/stop cycle through management on an acceptor bound to anauto-start=falsecoordinator), both failing without the respective fix, plus a unit test for the flag and parser coverage. The first commit compiles and its test passes on its own.